Skip to content

IBX-9810: Fixed inner validateProperty*() calls of StructWrapperValidator #521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 23, 2025
Merged

IBX-9810: Fixed inner validateProperty*() calls of StructWrapperValidator #521

merged 4 commits into from
Apr 23, 2025
+40 −7

Conversation

vidarl
Copy link
Contributor

@vidarl vidarl commented Apr 4, 2025 •
edited
Loading

🎫 Issue IBX-9810

Description:

Looks like a a cut&paste mistake when implementing the validateProperty() and validatePropertyValue() methods.

For QA:

Documentation:

@vidarl vidarl requested a review from Steveb-p April 4, 2025 06:43
@vidarl vidarl changed the base branch from main to 4.6 April 4, 2025 06:44
@vidarl vidarl requested a review from a team April 4, 2025 06:44
alongosz
alongosz reviewed Apr 4, 2025
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like something that is missing some test coverage.

@alongosz alongosz changed the title IBX-9810: New StructWrapperValidator breaks validateProperty() and validatePropertyValue() IBX-9810: Fixed inner validateProperty*() calls of StructWrapperValidator Apr 4, 2025
@vidarl
Copy link
Contributor Author

vidarl commented Apr 4, 2025

Seems like something that is missing some test coverage.

You can say that. But the Validator was never supposed to change behavior of the mentioned methods, only validate()
So the tests only tests validate() method

alongosz
alongosz reviewed Apr 4, 2025
View reviewed changes
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like something that is missing some test coverage.

You can say that. But the Validator was never supposed to change behavior of the mentioned methods, only validate() So the tests only tests validate() method

The fact that now we have a customer request indicates that this is a part of a system, which, while simple, requires some test coverage. Lack on such tests on a consumption of Symfony components creates quite significant issue for me when I try to bump major Symfony version. If you don't add those tests, the next customer request might be about this not working again in Ibexa DXP 5.0, because there will be no mechanism to detect that. Ofc if that happens, that's gonna be for a different reason.

A proper bug fix should always include some test coverage, unless for some reason not possible. We need to improve on that.

I wasn't asking for much in the first place - just a usage that calls the code in question, with mocks. Given you've found existing test case, turns out I'm asking for even less - just add test coverage for missing methods, please.

@vidarl vidarl force-pushed the IBX-9810_New_StructWrapperValidator_breaks_validateProperty_and_validatePropertyValue branch from 6aa89da to 7bad468 Compare April 7, 2025 06:22
@vidarl
Copy link
Contributor Author

vidarl commented Apr 7, 2025

Tests added in 7bad468

@vidarl vidarl requested a review from alongosz April 7, 2025 06:24
@vidarl
Copy link
Contributor Author

vidarl commented Apr 15, 2025

Thank you for the approvals.
You suggest we do full QA on this, or just merge ?
@adamwojs

@vidarl vidarl force-pushed the IBX-9810_New_StructWrapperValidator_breaks_validateProperty_and_validatePropertyValue branch from 42c15b1 to c9027d1 Compare April 16, 2025 06:08
@vidarl vidarl requested a review from konradoboza April 16, 2025 06:08
konradoboza
konradoboza reviewed Apr 22, 2025
View reviewed changes
Steveb-p
Steveb-p approved these changes Apr 22, 2025
View reviewed changes
@vidarl @Steveb-p
Apply suggestions from code review
28dcd29 
Co-authored-by: Paweł Niedzielski <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

This was referenced Apr 22, 2025
Closed
Closed
micszo
micszo approved these changes Apr 22, 2025
View reviewed changes
Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified with regression tests.

@adamwojs adamwojs merged commit 42a1259 into 4.6 Apr 23, 2025
22 checks passed
@adamwojs adamwojs deleted the IBX-9810_New_StructWrapperValidator_breaks_validateProperty_and_validatePropertyValue branch April 23, 2025 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@konradoboza konradoboza konradoboza approved these changes

@adamwojs adamwojs adamwojs approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

@micszo micszo micszo approved these changes

@tbialcz tbialcz tbialcz approved these changes

@alongosz alongosz Awaiting requested review from alongosz

Assignees

@mikadamczyk mikadamczyk

@alongosz alongosz

@tischsoic tischsoic

Labels
QA approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@vidarl @adamwojs @Steveb-p @alongosz @micszo @konradoboza @tbialcz @mikadamczyk @tischsoic